Hi @aviralgarg05. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aviralgarg05
Once this PR has been reviewed and has the lgtm label, please assign dprotaso for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.20%. Comparing base (2d3f345) to head (ea18c4a).
⚠️ Report is 4 commits behind head on main.

@@            Coverage Diff             @@
##             main   #16355      +/-   ##
==========================================
- Coverage   80.23%   80.20%   -0.03%     
==========================================
  Files         216      216              
  Lines       13440    13450      +10     
==========================================
+ Hits        10784    10788       +4     
- Misses       2293     2298       +5     
- Partials      363      364       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hi @aviralgarg05 ,
thanks for the PR, this topic is a bit more complicated though as the PR is only one side of solving this problem.
The other side is net-... implementations like https://github.com/knative-extensions/net-kourier that follow the specification to actually add the headers to the requests they route to the knative service.

I'm not experienced enough to know extending the spec would work especially considering that adding headers might not be supported by all implementations, I guess @dprotaso has more background on what could be done?

/hold

We don't want to accept spec changes without a discussion. Generally it requires us to follow the feature track process

https://github.com/knative/community/blob/main/mechanics/FEATURE-TRACKS.md

Also note the change as is has security concerns because you're able to set any arbitrary header which can mess up scale to zero activation.

So can you guide me on how we will be moving forward with this PR?

@aviralgarg05 frankly I'm unsure.

The other issue for me is expanding on our Route type might not be worth it if we can sort out how we can leverage HTTPRoute to point to Knative Revisions directly.

Thanks for the feedback @dprotaso.

I understand the concerns regarding security and the potential shift towards HTTPRoute.

Since expanding Route might not be the right direction, could you point me to any tracking
issue or discussion regarding the HTTPRoute integration? I'd be happy to help contribute to
that effort if it solves the underlying need for header customization.

Alternatively, if the HTTPRoute transition is further out, would you be open to a more
restricted version of this PR (e.g., allow-listing specific headers) to address the immediate
use case safely?

Let me know how you'd prefer to proceed.

It seems the work that would enable this has sorta stalled out upstream - kubernetes-sigs/gateway-api#3608

Alternatively, if the HTTPRoute transition is further out, would you be open to a more
restricted version of this PR (e.g., allow-listing specific headers) to address the immediate
use case safely?

I think it's probably best for me to get more information. For example if you're baking a fixed header and value in every incoming request is there a reason why this can't be configured in the application? eg. just set that option as an environment variable etc.

It seems the work that would enable this has sorta stalled out upstream - kubernetes-sigs/gateway-api#3608

Alternatively, if the HTTPRoute transition is further out, would you be open to a more
restricted version of this PR (e.g., allow-listing specific headers) to address the immediate
use case safely?

I think it's probably best for me to get more information. For example if you're baking a fixed header and value in every incoming request is there a reason why this can't be configured in the application? eg. just set that option as an environment variable etc.

That's a fair point! The reason we can't do it in the app is that these headers are for security tools that check the request before it even arrives. Since those tools see the request first, the app doesn't have a chance to add them.

I’m also definitely open to the restricted version you suggested if you think that's a safer way to go!

Also what networking layer are you using?

That's a fair point! The reason we can't do it in the app is that these headers are for security tools that check the request before it even arrives. Since those tools see the request first, the app doesn't have a chance to add them.

Can you clarify and expand on that? If you're configuring this security tool to check for these headers and to address this you want to add headers to the inbound request then that seems like you're introducing the constraint on yourself?

Also what networking layer are you using?

That's a fair point! The reason we can't do it in the app is that these headers are for security tools that check the request before it even arrives. Since those tools see the request first, the app doesn't have a chance to add them.

Can you clarify and expand on that? If you're configuring this security tool to check for these headers and to address this you want to add headers to the inbound request then that seems like you're introducing the constraint on yourself?

I am using Istio. The security tool is shared infrastructure that requires these headers to identify the application and apply the right policies. Since I don't manage that service directly, adding headers at the Route level is the best way for me to handle the integration

The security tool is shared infrastructure that requires these headers to identify the application and apply the right policies.

What's the tool?

The security tool is shared infrastructure that requires these headers to identify the application and apply the right policies.

What's the tool?

The original issue (#16298) describes it as an Istio External Authorizer handling SSO/JWT/SAML. I'm working from that use case rather than a specific deployment, so I don't have the exact tool name—just that it's a shared ExtAuthz service that uses headers like x-accept-auth to determine which auth flow to apply.

Thanks for the change. But I'm going to close this out as the original issue is actually a big security hole - please see: #16298 (comment)